Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add a pre-commit hook config file #244

Merged
merged 12 commits into from
Jan 16, 2021

Conversation

neutrinoceros
Copy link
Contributor

@neutrinoceros neutrinoceros commented Jan 13, 2021

Description

Adding this file to the repo makes vulture usable as a hook for pre-commit.
As such, it can be easily integrated to a shared development workflow and to CI (in particular within pre-commit.ci)

Related Issue

none

Checklist:

  • I have updated the documentation in the README.md file or my changes don't require an update.
  • I have added an entry in CHANGELOG.md.
  • I have added or adapted tests to cover my changes.

@neutrinoceros
Copy link
Contributor Author

I was able to test this hook locally and did not encounter any obstacle.
Screen Shot 2021-01-13 at 19 15 09

Important note: in my addition to README.md, I'm pointing to what I think is the next logical release for vulture (2.2), but this is only relevant if said release is done immediately following this merge. If there's a better way to accommodate your workflow please tell me !

@jendrikseipp
Copy link
Owner

Thanks! The code looks good, I modified it slightly though. However, I'm not sure how useful the pre-commit integration will be, because it will test modified files and maybe even only one file at a time. Can you test this and confirm these two things, please?

Is there a way to always check all Python files tracked by the Git repo?

Also, can you check that the pre-commit invocation really uses the Vulture configuration from the pyproject.toml file?

@neutrinoceros
Copy link
Contributor Author

it will test modified files and maybe even only one file at a time. Can you test this and confirm these two things, please?

I confirm that it checks only modified files (unless run with --all-files flag as in my screenshot above, which is also what pre-commit-ci does underneath), but it catches them all: not just one by one.

Is there a way to always check all Python files tracked by the Git repo?

I'm pretty sure there is, but I need to inquire this further. I'll get back at you !

Also, can you check that the pre-commit invocation really uses the Vulture configuration from the pyproject.toml file?

Yup. Tested on a large (and old) repo, and I confirm that my configuration is correctly used. This is what I used:

# pyproject.toml
[tool.vulture]
min_confidence = 100

with this conf
Screen Shot 2021-01-13 at 20 27 32

and without it
Screen Shot 2021-01-13 at 20 27 55

(both screen shots are cut off, you get the point)

@neutrinoceros
Copy link
Contributor Author

Is there a way to always check all Python files tracked by the Git repo?

So I haven't worked my head around it yet but as a note the hook for https://github.com/asottile/dead does it (asottile is also the author of pre-commit), so it's definitely possible !

Writing this I am also realising I may have misinterpreted your question about testing files one by one... of course you meant to check wether vulture is still able to connect the dots if something is unused in the file where it's defined but imported elsewhere. I think that indeed it only looks at the subset of modified files so one gets pretty garbage reports in that state (which I didn't realise because everything looks good when the min confidence is set to 100%).

@neutrinoceros
Copy link
Contributor Author

Here is the mechanism Anthony used to systematically run dead against all files:
https://github.com/asottile/dead/blob/b3ca1d3bed59300d4bd7c61b76273368b569cddf/dead.py#L212

Replicating this feature in vulture would obviously require a deeper patch.
Dead has the luxury of assuming it's run in a git repo, but vulture doesn't.
The cleanest option that wouldn't break backward compatibility I'm seeing is adding an argument to vulture (e.g. --git, or something clearer ?) which could be passed instead of a file list. What do you think ?

@pquentin
Copy link

The cleanest option that wouldn't break backward compatibility I'm seeing is adding an argument to vulture (e.g. --git, or something clearer ?) which could be passed instead of a file list. What do you think ?

I'm only a vulture user migrating to pre-commit, but that's one honking great idea!

@jendrikseipp
Copy link
Owner

I think another option is to use pass_filenames: false for pre-commit (https://pre-commit.com/#hooks-pass_filenames) and define the filenames in Vulture's pyproject.toml file. Could you try if that works and adapt the PR accordingly if it does, please? Probably, we also need to set require_serial: true since otherwise I guess multiple Vulture instances might run.

…d pass_files: false so that vulture can detect files itself
@neutrinoceros
Copy link
Contributor Author

define the filenames in Vulture's pyproject.toml file.

I don't know how one would do this. Maybe I'm just confused. Could you elaborate ?

The rest (pass_filenames and require_serial) look absolutely necessary indeed. I overlooked the serial/parallel aspect, thanks !

@jendrikseipp
Copy link
Owner

define the filenames in Vulture's pyproject.toml file.

I don't know how one would do this. Maybe I'm just confused. Could you elaborate ?

I mean that we should add something along the following lines to the README:

To run Vulture before each commit, add the following pre-commit hook to name of yaml file and specify all files that should be checked in the pyproject.toml file under the paths key.

@codecov-io
Copy link

codecov-io commented Jan 15, 2021

Codecov Report

Merging #244 (594d6a2) into master (fdac4e9) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #244   +/-   ##
=======================================
  Coverage   99.36%   99.36%           
=======================================
  Files          17       17           
  Lines         631      631           
=======================================
  Hits          627      627           
  Misses          4        4           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fdac4e9...594d6a2. Read the comment docs.

@neutrinoceros
Copy link
Contributor Author

Thanks (and done) ! I can also confirm after testing this setup locally that it works like a charm !
I see you just released vulture 2.2 so I'm bumping the suggested version number to use to 2.3 :)

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@jendrikseipp jendrikseipp merged commit cb5e882 into jendrikseipp:master Jan 16, 2021
@jendrikseipp
Copy link
Owner

Thanks!

@neutrinoceros neutrinoceros deleted the pre-commit-hook branch January 16, 2021 13:52
@neutrinoceros
Copy link
Contributor Author

Thank you for merging ! can you please bump Vulture's version to 2.3 ?

@jendrikseipp
Copy link
Owner

Done :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants